Skip to content

Conversation

FRASTM
Copy link
Contributor

@FRASTM FRASTM commented Sep 22, 2025

Define the STM32_CK48_ENABLED especially for the stm32F4 series

FIxes #94934

@FRASTM FRASTM changed the title stminclude: drivers: stm32 clock mux CK48 definition stm32 clock mux for CK48 definition Sep 22, 2025
@FRASTM FRASTM force-pushed the issue94934 branch 2 times, most recently from d4d97b6 to 6c38115 Compare September 22, 2025 15:44
@FRASTM FRASTM marked this pull request as ready for review September 23, 2025 06:59
Comment on lines +19 to +34
&pll {
div-m = <4>;
mul-n = <192>;
div-p = <4>;
div-q = <8>;
clocks = <&clk_hse>;
status = "okay";
};

&rcc {
clocks = <&pll>;
clock-frequency = <DT_FREQ_M(96)>;
ahb-prescaler = <1>;
apb1-prescaler = <2>;
apb2-prescaler = <1>;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional change?
Would it be worth to add an alternate test with its own DTS overlay file?

Copy link
Contributor Author

@FRASTM FRASTM Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, when the sdmmc is clocked by the PLL_q, a freq = 48MHz is required.
This overlay file "f4_sdmmc48_pll.overlay"is for running this testcase by un-commenting.
The alternate test is "drivers.clock.stm32_clock_configuration.common_device.f4.sdmmc_48".
Two possible config are in this overlay (selection is by commenting/un-commenting). Is there any reason today to change with 2 different testcases/overlay files, if it was your question ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fine. Already having a test for SDMMC over CK48 seems enough.
I just found strange this added inline comments in this commit.

@FRASTM FRASTM added the platform: STM32 ST Micro STM32 label Oct 8, 2025
mathieuchopstm
mathieuchopstm previously approved these changes Oct 9, 2025
etienne-lms
etienne-lms previously approved these changes Oct 9, 2025
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Maybe the commit message could be more verbose on why the change, e.g.:

 Define the STM32_CK48_ENABLED especially for the stm32F4 series
+when ck48 node is enabled to leverage its already implemented support.

Comment on lines +19 to +34
&pll {
div-m = <4>;
mul-n = <192>;
div-p = <4>;
div-q = <8>;
clocks = <&clk_hse>;
status = "okay";
};

&rcc {
clocks = <&pll>;
clock-frequency = <DT_FREQ_M(96)>;
ahb-prescaler = <1>;
apb1-prescaler = <2>;
apb2-prescaler = <1>;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fine. Already having a test for SDMMC over CK48 seems enough.
I just found strange this added inline comments in this commit.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nack until #94934 (comment) is fixed

@erwango erwango assigned erwango and unassigned nordic-krch Oct 13, 2025
@FRASTM FRASTM dismissed stale reviews from etienne-lms and mathieuchopstm via efda26e October 13, 2025 07:56
@FRASTM
Copy link
Contributor Author

FRASTM commented Oct 13, 2025

Add a commit to enable the clk48

@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Oct 13, 2025
@FRASTM FRASTM added this to the 4.3 milestone Oct 13, 2025
@FRASTM
Copy link
Contributor Author

FRASTM commented Oct 14, 2025

LGTM.

Maybe the commit message could be more verbose on why the change, e.g.:

 Define the STM32_CK48_ENABLED especially for the stm32F4 series
+when ck48 node is enabled to leverage its already implemented support.

comment updated

etienne-lms
etienne-lms previously approved these changes Oct 14, 2025
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment below is not blocking.

@etienne-lms
Copy link
Contributor

@FRASTM, I think you need to rebase to get the CI tests to all succeed.

@mathieuchopstm mathieuchopstm modified the milestones: 4.3, v4.3.0 Oct 14, 2025
Define the STM32_CK48_ENABLED especially for the stm32F4 series
when ck48 node is enabled to leverage its already implemented support.

Signed-off-by: Francois Ramu <[email protected]>
@FRASTM
Copy link
Contributor Author

FRASTM commented Oct 15, 2025

rebase on 9c5325d

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erwango, could you update your review status? You #96353 (review) comment was addressed: see #94934 (comment).

Copy link

erwango
erwango previously approved these changes Oct 15, 2025
This commit enables the clk48 clock mux if STM32_CK48_ENABLED
is set by the device tree.

Signed-off-by: Francois Ramu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Clock Control area: Tests Issues related to a particular existing or missing test platform: STM32 ST Micro STM32

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stm42f412 "Failed to enable SDMMC domain clock"

6 participants